Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#397 - constrained_dimensions #398

Merged
merged 7 commits into from
Jan 22, 2019
Merged

#397 - constrained_dimensions #398

merged 7 commits into from
Jan 22, 2019

Conversation

kpotomkin
Copy link
Collaborator

@kpotomkin kpotomkin commented Nov 28, 2018

Closes #397.

@kpotomkin kpotomkin self-assigned this Nov 28, 2018
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
@schillic
Copy link
Member

Please see my comment here. We want to have a map from location to variables.

Also, please use Int instead of Int64 😉

@mforets
Copy link
Member

mforets commented Nov 28, 2018

Also, please use Int instead of Int64

Int is platform-independent.

src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
@mforets
Copy link
Member

mforets commented Nov 29, 2018

on the function's name: get_necessary_vars -> nonzero_vars?

@schillic
Copy link
Member

on the function's name: get_necessary_vars -> nonzero_vars?

I would rather say constrained_dims or constrained_dimensions, even if for now the function does not work for general sets.
Actually I would say that we should have such a function in LazySets because not every set supports constraints_list and for some sets this functionality can be achieved cheaper than computing the constraints first.

@mforets
Copy link
Member

mforets commented Nov 30, 2018

👍 for outsourcing constrained_dimension to LazySets. This proposes to change

...
       for constraint in constraints_list(stateset(HS, mode))
            append!(vars, findall(!iszero, constraint.a))
       end
...

to

     append!(vars, constrained_dims(stateset(HS, mode))

@schillic
Copy link
Member

I start losing the overview 😟
JuliaReach/LazySets.jl#766
JuliaReach/LazySets.jl#792

@mforets
Copy link
Member

mforets commented Nov 30, 2018

if constrained_dims(::LazySet) (or another speific set type is defined in LazySets), it is still releveant to add (outside LazySets) a methods constrained_dims(::HybridSystem) or with a similar name.

@schillic
Copy link
Member

it is still releveant to add (outside LazySets) a methods constrained_dims(::HybridSystem) or with a similar name

Why? We will not use it (we need this information per location).

@mforets
Copy link
Member

mforets commented Nov 30, 2018

maybe, but i'm not sure: if this function is used to reason about the options to be passed to an algorithm (eg. to decide which blocks of variables are selected), one can call it over the given HybridSystem as a preprocessing step.

@schillic
Copy link
Member

We now support constrained_dimensions(::HPolyhedron), so you can refactor that part.

@mforets
Copy link
Member

mforets commented Dec 5, 2018

  • i'm still not conviced about the name get_necessary_vars, what do you think about constrained_dimensions(::HybridSystem)?

  • we should add an example to test this function. we can add it to solve_hybrid_bouncing_ball.jl and solve_hybrid_thermostat.jl

src/solve.jl Show resolved Hide resolved
@schillic
Copy link
Member

  • we should add an example to test this function. we can add it to solve_hybrid_bouncing_ball.jl and solve_hybrid_thermostat.jl

I thought to have this function private, so we can call it only inside solve. In addition, it will be very 'artificial' test, which will not show real value of this function.

Private functions should also be tested. I do not see why the test would be artificial. You can very well predict and describe the outcome of this function.
I think what @mforets meant with the examples was that you can use the automata created in there. (however, they are boring). In #404 we would then separate these tests again.

@mforets
Copy link
Member

mforets commented Dec 11, 2018

I think what @mforets meant with the examples was that you can use the automata created in there. (however, they are boring).

Yes! While we don't have #404, we can just add a call to constrained_dimensions(::HS) in each model file, then @test that the function behaves as expected.

@mforets
Copy link
Member

mforets commented Jan 18, 2019

Bump?

I think that it just remains to test this file with bouncing ball and thermostat.

@schillic schillic changed the title [WIP] 397 - initial implementation of get_necessary_vars #397 - constraint_dimensions Jan 22, 2019
@schillic schillic changed the title #397 - constraint_dimensions #397 - constrained_dimensions Jan 22, 2019
@schillic schillic merged commit 6340067 into master Jan 22, 2019
@schillic schillic deleted the 397 branch January 22, 2019 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants